Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: improve mfa.enroll return types #956

Merged
merged 1 commit into from
Sep 17, 2024
Merged

fix: improve mfa.enroll return types #956

merged 1 commit into from
Sep 17, 2024

Conversation

aloisklink
Copy link
Contributor

What kind of change does this PR introduce?

Bug fix for the TypeScript types.

What is the current behavior?

What is the new behavior?

Use TypeScript's function overloading to return

  • TOTP data when mfa.enroll was passed factorType: 'totp',
  • phone data when mfa.enroll was passed factorType: 'phone'.

This fixes a breaking change in the types in b957c30.

Additional context

This PR would also expose the new following types:

  • MFAEnrollTOTPParams
  • MFAEnrollPhoneParams
  • AuthMFAEnrollTOTPResponse
  • AuthMFAEnrollPhoneResponse
  • AuthMFAEnrollErrorResponse (it might be better to roll this type in with the other two).

It might be better to keep these types private, since once they're exposed, you can't easily remove them again without creating a breaking change.

@aloisklink
fix: improve mfa.enroll return types
01ba4ff 
Use TypeScript's operator overloading to return
- TOTP data when `mfa.enroll` was passed `factorType: 'totp'`,
- phone data when `mfa.enroll` was passed `factorType: 'phone'`.

This fixes a breaking change in the types in
b957c30.
@J0
Copy link
Contributor

J0 commented Sep 17, 2024

It might be better to keep these types private, since once they're exposed, you can't easily remove them again without creating a breaking change.

Would agree with this

Thanks for the PR! Looks great so far

@J0 J0 merged commit 8a1ec06 into supabase:master Sep 17, 2024
3 checks passed
@github-actions github-actions bot mentioned this pull request Sep 17, 2024
Merged
@J0
Copy link
Contributor

J0 commented Sep 17, 2024 •
edited
Loading

Will follow up with the corresponding PR for challengeParams and verifyParams (if they are at all needed)

Upd: shouldn't be needed, except in relation to MFA (WebAuthn).

@aloisklink
Copy link
Contributor Author

It might be better to keep these types private, since once they're exposed, you can't easily remove them again without creating a breaking change.

Would agree with this

If you are still interested in this, I think the easiest way to do this would be to make a new file called something like src/lib/internal-types.ts that isn't exported outside of the package using export * from './lib/types'.

Then, src/GoTrueClient.ts and src/lib/types.ts could both import types from this file.

(the hardest thing would be picking the right conventional commit prefix 😆, since this PR has already been merged, but not released, so it's not yet a breaking change)

Will follow up with the corresponding PR for challengeParams and verifyParams (if they are at all needed)

Upd: shouldn't be needed, except in relation to MFA (WebAuthn).

Yep, we use both supabase.auth.mfa.challenge and supabase.auth.mfa.verify in our code (based of the example in https://github.com/supabase/supabase/blob/10ba4999cd33f796e89c50a0821e46b3d346eb7c/apps/docs/content/guides/auth/auth-mfa/totp.mdx?plain=1#L245-L261), and v2.65.0 works fine in those cases!

And I'm excited to see WebAuthn support!

@aloisklink aloisklink deleted the fix/fix-mfa.enroll-return-types branch September 18, 2024 06:55
@J0
Copy link
Contributor

J0 commented Sep 23, 2024

@aloisklink, thanks for the update.

I think an internal-types file would work. Let me do that shortly. Thanks!

@J0 J0 mentioned this pull request Sep 27, 2024
Merged
J0 added a commit that referenced this pull request Oct 8, 2024
@J0
fix: move MFA sub types to internal file (#964)
4b7455c 
## What kind of change does this PR introduce?

Follow up to #956, hides the MFA sub-types (e.g. MFA Phone, MFA TOTP)
needed to fix type errors before we release to public so that we can
remove them as and when we like. The sub-types are now stored in an
`internal-types` file which, unlike the `types` file is not exposed to
public.


The types mentioned here have not been released yet so fine to hide them
kangmingtay pushed a commit that referenced this pull request Oct 15, 2024
@github-actions
chore(master): release 2.65.1 (#959)
1b6cdd2 
🤖 I have created a release *beep* *boop*
---


##
[2.65.1](v2.65.0...v2.65.1)
(2024-10-14)


### Bug Fixes

* Call `SIGNED_OUT` event where session is removed
([#854](#854))
([436fd9f](436fd9f))
* improve `mfa.enroll` return types
([#956](#956))
([8a1ec06](8a1ec06))
* move MFA sub types to internal file
([#964](#964))
([4b7455c](4b7455c))
* remove phone mfa deletion, match on error codes
([#963](#963))
([ef3911c](ef3911c))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@J0 J0 J0 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

mfa.enroll's return type has a TypeScript breaking change in v2.65.0
2 participants
@aloisklink @J0